With the usual caveat of

I had a brief play with this myself and got distracted with also doing the update bits too* - I couldn't see an easy way to do it without changing the API of ChecksPublisher to look something like:

public abstract class ChecksPublisher {

    private final Job<?, ?> job;

    public ChecksPublisher(Job<?, ?> job) {
        this.job = job;
    }

    protected abstract void doPublish(ChecksDetails details)  // replaces existing `publish`

    public final void publish(ChecksDetails details) {
        doPublish(details);
        details.getName().ifPresent(name -> job.addOrReplaceAction(new ChecksInfo(name, details.getConsclusion()));
    }

    public ChecksConclusion getConclusion(String checksName) {
        return job.getActions(ChecksInfo.class)
                .stream()
                .filter(a -> a.getName().equals(checksName))
                .findFirst()
                .map(ChecksInfo::getConclusion)
                .orElse(ChecksConclusion.None);
    }
}

ie, ChecksInfo was an InvisibleAction, and there was a separate WithChecksContext that stored the current name.

The WithChecksStep then implements a BodyExecutionCallback that looks something like:

private static final class Callback extends BodyExecutionCallback {
    private static final long serialVersionUID = 1L;

    private ChecksPublisher getPublisher(StepContext context) throws IOException, InterruptedException {
        return ChecksPublisherFactory.fromRun(context.get(Run.class), context.get(TaskListener.class));
    }

    private WithCheckContext getCheckContext(StepContext context) throws IOException, InterruptedException {
        return context.get(WithCheckContext.class);
    }

    private ChecksDetails getDetails(WithCheckContext context, ChecksConclusion conclusion) {
        return new ChecksDetails.ChecksDetailsBuilder()
                .withName(context.getName())
                .withConclusion(conclusion)
                .withStatus(ChecksStatus.COMPLETED)
                .build();
    }

    @Override
    public void onStart(StepContext context) {
        try {
            ChecksDetails details = new ChecksDetails.ChecksDetailsBuilder()
                    .withName(getCheckContext(context).getName())
                    .withStatus(ChecksStatus.IN_PROGRESS)
                    .build();
            getPublisher(context).publish(details);
        } catch (Exception e) {
            context.onFailure(e);
        }
    }

    @Override
    public void onSuccess(StepContext context, Object result) {
        try {
            WithCheckContext checkContext = getCheckContext(context);
            ChecksPublisher publisher = getPublisher(context);
            if (publisher.getConclusion(checkContext.getName()) == ChecksConclusion.NONE) {
                publisher.publish(getDetails(checkContext, ChecksConclusion.SUCCESS));
                context.onSuccess(null);
            }
        } catch (Exception e) {
            context.onFailure(e);
        }
    }

    @Override
    public void onFailure(StepContext context, Throwable t) {
        try {
            WithCheckContext checkContext = getCheckContext(context);
            ChecksPublisher publisher = getPublisher(context);
            if (t instanceof FlowInterruptedException && ((FlowInterruptedException) t).isActualInterruption()) {
                publisher.publish(getDetails(checkContext, ChecksConclusion.CANCELED));
            } else if (publisher.getConclusion(checkContext.getName()) != ChecksConclusion.FAILURE) {
                publisher.publish(getDetails(checkContext, ChecksConclusion.FAILURE));
            }
            context.onFailure(t);
        } catch (Exception e) {
            context.onFailure(e);
        }
    }
}

Obviously the API change is sub-optimal, but at least it only affects implementations of this API (of which we have full control :)) rather than consumers of it (which we do not :()

* it's somewhat scope creep, but I wondered if the ability to update a check should be an implementation detail consumers of the checks api plugin, or something provided by it - e.g. by storing the external id associated with a checks name in this new ChecksInfo class. If so, now might be the ideal time to do that.

Instead of storing the published checks locally, have you ever thought about query them from GitHub? Maybe use the commit: https://docs.github.com/en/free-pro-team@latest/rest/reference/checks#list-check-runs-for-a-git-reference.

This needs extra requests to GitHub, but seems not a not big deal?

Yeah, I think that would be neater for the most part - means that we're not attempting to mirror github's state and hoping we've got it right...

We would still need to add a new API to the publisher though, possibly, for onSuccess and onFailure to get the current state of the check; so if we add an

    abstract public ChecksConclusion getConclusion(String checksName)

to ChecksPublisher then implementations can either attempt to store that information locally, or fetch it from the external source.

This could, I guess, return ChecksConclusion.NONE by default...

Though the comment

Note: The Checks API only looks for pushes in the repository where the check suite or check run were created. Pushes to a branch in a forked repository are not detected and return an empty pull_requests array.

doesn't inspire me with confidence...

We would still need to add a new API to the publisher though, possibly, for onSuccess and onFailure to get the current state of the check;

But if we store the info of the check as an invisible action to the run, couldn't we directly resolve it in StepContext?

doesn't inspire me with confidence...

Yeah, seems a problem; I'll test it.

@mrginglymus I test and it actually lists check runs of the PR from a forked repo: XiongKezhi/codingstyle#13 \o/

I think the reason may be: the commits of a PR (filed from a forked repo or not) in the repository tracked by Jenkins are indeed the "pushes in the repository where the check suite or check run were created" as the GitHub doc mentions.

Do you have time to double-check it? Maybe in your repos.

I think the reason may be: the commits of a PR (filed from a forked repo or not) in the repository tracked by Jenkins are indeed the "pushes in the repository where the check suite or check run were created" as the GitHub doc mentions.

That does make sense (otherwise how would you create check runs) - but then I wonder what the note is about...

So as I see it there are two problems, each with the same two options:

1. Tracking the 'id' of an check so we can update it
   a. we could do this in the checks-api by storing a mapping of String name: long id
   b. we could defer this to the implementation, which could either do its own tracking of the id as in a., or it could look up the id on demand
2. Tracking the status of a check so we know whether or not it has been completed at the end of the withChecks block
   a. we could do this in the checks-api by storing a mapping of String name: ChecksConclusion conclusion
   b. we could defer this to the implementation, which could either do its own tracking of the status as in a., or it could look up the conclusion on demand

For 1., I think b. (doing it within the implementation) seems like it might make more sense as it allows each implementation to have its own understanding of what 'updating' actually means, and minimises api changes for checks-api - you just continue to call publish and it's up to the implementation to work out if that means an update or a create.

For 2., I think b. also might make sense, again in the interest of minimising changes to the API of the checks-api - just adding a getConclusion(String name) to the ChecksPublisher interface would allow us to implement everything necessary here. We can then decide later on whether we want to track the id/conclusions in InvisibleActions or by fetching on demand, without having to change all the APIs.

I might throw up a quick PR for github-checks-api just to put into code the above waffle, as I think it'll make more sense :D

edit: example PR here: jenkinsci/github-checks-plugin#87

cc @mrginglymus (cannot request your review directly in the reviewers, maybe because you are not in the jenkins org)

Snippet Generator Preview:

Hooray!

This is great, and will hopefully make it a lot easier to have all the plugins have overridable checks names without complicated configuration options on each.

Should we add support for withChecks to publishChecks? As mentioned in other comments, I'm hoping to have things like:

withChecks('Python Tests') {
    publishChecks status: IN_PROGRESS
    try {
        sh 'pytest tests'
        junit 'results.xml'
    } catch (FlowInterruptedException err) {
        publishChecks status: CANCELLED
        throw err
   } catch( Exception err) {
       publishChecks status: FAILED
       throw err
   }
}

It would also help serve as a guide to developers on how to use it.

Do you think that the above pattern might be something we could build into the withChecks step? AFICT it should be doable by implementing a BodyExecutionCallback listener, though I appreciate that perhaps it's too much heavy lifting for a single command. Maybe as a further extension in a separate PR?

Should we add support for withChecks to publishChecks?

I think we should.

Do you think that the above pattern might be something we could build into the withChecks step? Maybe as a further extension in a separate PR?

I think we should do that and it's ok to do it in this PR? @timja

And if we do that, we will set the name as a mandatory field, or we don't know what to publish when the exceptions occur.
So for double check, Do you think there will be situations where you do not want to specify the name but only the status (e.g. publish several checks in the same status (say in progress) but with different names in the closure)?

Should we add support for withChecks to publishChecks?

I think we should.

Do you think that the above pattern might be something we could build into the withChecks step? Maybe as a further extension in a separate PR?

I think we should do that and it's ok to do it in this PR? @timja

Yes sounds good

And if we do that, we will set the name as a mandatory field, or we don't know what to publish when the exceptions occur.
So for double check, Do you think there will be situations where you do not want to specify the name but only the status (e.g. publish several checks in the same status (say in progress) but with different names in the closure)?

If you want different names then use a different closure

What is the expected behaviour if you nest withChecks calls? Presumably it's something we don't support (and should explicitly 💥 on)?

Seems the injected ChecksInfo cannot be accessed inside the callback? @timja @mrginglymus

context.get(ChecksInfo.class) returns nothing.

I can directly pass the ChecksInfo to the callback class while constructing if that is the case.

Not sure